Skip to content

Fix Go client data races#586

Merged
patniko merged 5 commits intogithub:mainfrom
chlowell:go-race
Feb 27, 2026
Merged

Fix Go client data races#586
patniko merged 5 commits intogithub:mainfrom
chlowell:go-race

Conversation

@chlowell
Copy link
Contributor

@chlowell chlowell commented Feb 25, 2026

The client's running bool field is accessed concurrently by Stop() and readLoop() methods without synchronization, creating a data race. This can be prevented by simply changing the field's type to atomic.Bool. I also enabled the go test race detector in the test script so future CI runs will fail when the test suite triggers a race.

@chlowell chlowell marked this pull request as ready for review February 25, 2026 22:28
@chlowell chlowell requested a review from a team as a code owner February 25, 2026 22:28
Copilot AI review requested due to automatic review settings February 25, 2026 22:28
Copilot AI reviewed Feb 25, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a data race in the Go JSON-RPC client by converting the running field from a plain bool to atomic.Bool. The race occurred because the field was accessed concurrently by the Stop() method and the background readLoop() goroutine without synchronization. The PR also enables the Go race detector in the test script to catch such issues in future CI runs.

Changes:

  • Changed running field from bool to atomic.Bool and updated all access points to use atomic operations (Store(), Load())
  • Enabled the -race flag in go test command to detect data races in CI

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
go/test.sh Added -race flag to enable Go's race detector during test execution
go/internal/jsonrpc2/jsonrpc2.go Fixed data race by converting running field to atomic.Bool and using atomic operations for all accesses

@SteveSandersonMS
Copy link
Contributor

SteveSandersonMS commented Feb 26, 2026

Thanks for contributing this, @chlowell!

I notice that CI is failing with this change due to it detecting another data race:

==================
    testing.go:1490: race detected during execution of test
=== RUN   TestRpc/should_call_RPC.Models.List_with_typed_result
    rpc_test.go:64: Not authenticated - skipping models.list test
==================
WARNING: DATA RACE
Write at 0x00c000176228 by goroutine 325:
  github.com/github/copilot-sdk/go.(*Client).ForceStop()
      /Users/runner/work/copilot-sdk/copilot-sdk/go/client.go:386 +0x110
  github.com/github/copilot-sdk/go/internal/e2e.TestRpc.func2.1()
      /Users/runner/work/copilot-sdk/copilot-sdk/go/internal/e2e/rpc_test.go:52 +0x2c
  testing.(*common).Cleanup.func1()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1211 +0x140
  testing.(*common).runCleanup()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1445 +0x1e0
  testing.tRunner.func2()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1786 +0x48
  runtime.Goexit()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/runtime/panic.go:636 +0x5c
  testing.(*common).Skip()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1132 +0x78
  github.com/github/copilot-sdk/go/internal/e2e.TestRpc.func2()
      /Users/runner/work/copilot-sdk/copilot-sdk/go/internal/e2e/rpc_test.go:64 +0x2dc
  testing.tRunner()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /Users/runner/hostedtoolcache/go/1.24.13/arm64/src/testing/testing.go:1851 +0x40

Previous read at 0x00c000176228 by goroutine 327:
  github.com/github/copilot-sdk/go.(*Client).startCLIServer.func1()
      /Users/runner/work/copilot-sdk/copilot-sdk/go/client.go:1102 +0x34

from https://github.com/github/copilot-sdk/actions/runs/22418728180/job/65004395091?pr=586

If we're adding this detection, could you also update other places that would trigger it? Thanks!

@chlowell
Copy link
Contributor Author

chlowell commented Feb 26, 2026

Sorry I missed that one; I haven't been able to repro it on my machine with the current test suite. I added a test that reliably triggers it and other races in copilot.Client, which I've now fixed. I also fixed some process leaks that came up while I was working on synchronization. The new synchronization is a bit involved because some data shared by Start and [Force]Stop is already synchronized and a simple mutex covering all three methods won't work because ForceStop needs to function concurrently with Start and Stop. I was aiming for a smallish fix; let me know what you think.

@chlowell chlowell changed the title Fix Go JSON-RPC client data race Fix Go client data races Feb 26, 2026
@patniko patniko added this pull request to the merge queue Feb 27, 2026
Merged via the queue into github:main with commit 5a4f823 Feb 27, 2026
16 checks passed
@chlowell chlowell deleted the go-race branch February 27, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@patniko patniko patniko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chlowell @SteveSandersonMS @patniko